Skip to content

Conversation

bricknerb
Copy link
Contributor

Instead of having a switch case with fallthrough where each case adds the number of entities of the next id type case, have an array that maps id type to a function that gets the number of the matching entities.
This array is less confusing and easier to maintain to avoid bugs like the one fixed in #6151.
The logic that sums only the entities above the given id is now a common logic so no need to change it when changing the set of ids.

…end and maintain

Instead of having a switch case with `fallthrough` where each case adds the number of entities of the next id type case, have an array that maps id type to a function that gets the number of the matching entities.
This array is less confusing and easier to maintain to avoid bugs like the one fixed in carbon-language#6151.
The logic that sums only the entities above the given id is now a common logic so no need to change it when changing the set of ids.
@bricknerb bricknerb marked this pull request as ready for review October 2, 2025 18:54
@bricknerb bricknerb requested a review from a team as a code owner October 2, 2025 18:54
@bricknerb bricknerb requested review from jonmeow and removed request for a team October 2, 2025 18:54
Copy link
Contributor

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels pretty subjective... for me personally, this looks worse.

@jonmeow
Copy link
Contributor

jonmeow commented Oct 2, 2025

To clarify my comment, before, the code block was:

      offset += sem_ir_->associated_constants().size();
      [[fallthrough]];
    case ScopeIdTypeEnum::For<AssociatedConstantId>:

Now it looks like the code block is:

      {ScopeIdTypeEnum::For<AssociatedConstantId>,
       [](const File* sem_ir) {
         return sem_ir->associated_constants().size();
       }},

Both of these seem similar to me, with a couple differences; I always find lambdas add complexity for me, but also the new code has more general framework (i.e. more to understand).

So where you say: "The logic that sums only the entities above the given id is now a common logic so no need to change it when changing the set of ids." I don't see how the two are much different: there's a code block for a given group, and additions need an addition.

I completely understand that you may find it easier to reason about, but to me the addition of a struct and complex control flow is not fundamentally easier to reason about. If you want something simpler, I'd suggest a solution that drops both the struct and lambdas.

Comment on lines -138 to -140
offset += sem_ir_->cpp_overload_sets().size();
[[fallthrough]];
case ScopeIdTypeEnum::For<CppOverloadSetId>:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we grouped these things with whitespace so that you saw the things above the case are related to it more clearly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created #6165.

github-merge-queue bot pushed a commit that referenced this pull request Oct 6, 2025
…tNamer::GetScopeIdOffset()` (#6165)

This would hopefully help prevent bugs like the one fixed in #6151.
See refactoring discussion in #6159.
@bricknerb bricknerb closed this Oct 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants